Use Vector4 storage for tangents in SurfaceTool for consistency - #119145
Conversation
The interface SurfaceTool presents for tangents and all its usages work with tangent frames represented as 4 values: tangent direction and scalar orientation (+1 or -1). However, internally it was represented as tangent+bitangent (mistakenly called binormal), which introduces additional conversions back & forth and is problematic for replacing the tangent space generation algorithm. This change converts the internal storage and adjusts the calling code accordingly. Note that since SurfaceTool::set_tangent is exposed to scripts, I've kept the Plane interface there even though it's misleading to use this type here.
d642e1f to
d00e574
Compare
Vector4 didn't exist in Godot until recently! For Godot 5 we will change the user-facing API to use Vector4 here as well
I don't have a problem with something like this. We have a similar pattern elsewhere. You could do something like:
|
|
Thanks! |
…ector4 Use Vector4 storage for tangents in SurfaceTool for consistency
What does this do
The interface SurfaceTool presents for tangents and all its usages work with tangent frames represented as 4 values: tangent direction and scalar orientation (+1 or -1).
However, internally it was represented as tangent+bitangent (mistakenly called binormal), which introduces additional conversions back & forth and is problematic for replacing the tangent space generation algorithm.
This change converts the internal storage and adjusts the calling code accordingly. Note that since
SurfaceTool::set_tangentis exposed to scripts, I've kept the Plane interface there even though it's misleading to use this type here (it does not describe the type correctly, I can only imagine this was used because Vector4 wasn't exposed to scripts?)Note about the bitangent flipping code: this was needed because there is a disagreement between MikkTSpace and Godot on face winding (CW vs CCW). As a result, MikkTSpace internally uses flipped face normals compared to what Godot expects. This is mostly benign - it does not affect internal math etc. - but it does mean that, when bitangents were reprojected, they would flip compared to the expected direction of cross product according to the orientation flag. Using the orientation flag directly produces the same result and does not require coordinate system based flipping.
Tested on a few meshes including MRP from #114508, using a patch to visualize bitangent in the shader to validate that this change does not change generated tangent frames.
Why do we need this
Everywhere else in Godot, tangent frames are a 4-component vector (although sometimes encoded in a Plane for some reason), so I'd argue this is just good for consistency, and it slightly shrinks the SurfaceTool::Vertex which slightly speeds up reindexing operations, in addition to making it easier to reason about data propagation through the pipeline.
But the actual reason why I need this is that I have patches that, 1) replace mikktspace.c based tangent generation with new meshoptimizer tangent generation, 2) fix the tangent splitting on mirrored UVs (#114508). These simultaneously improve tangent quality and make tangent space generation ~6-7x faster on import, and using Vector4 in SurfaceTool helps maximize performance there as we no longer need to reconstruct bitangent just to later use it to recover the orientation. Since it is an independent cleanup that doesn't rely on a future version of meshoptimizer I figured I could submit that separately first.